-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Avoid name clashing #2
base: master
Are you sure you want to change the base?
Conversation
@bboure did you have any chance to review this pr? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @mstn and sorry for late review. I must have missed it.
As you can tell, I haven't been working on this for a long time.
In what case would there be a clash in the naming?
Is this in the case a domain already exists with the same name?
if so, wouldn't this line re-use it?
If I remember well, the idea was to be able to manage an existing domain.
Do you see a reason why this would be a bad idea?
and Thank you for your contrib!
const prevEbsOptions = pick(prevDomain.ebsOptions, ['EBSEnabled', 'VolumeSize', 'VolumeType']); | ||
|
||
const clusterConfig = pick(domain.elasticsearchClusterConfig, ['InstanceCount', 'InstanceType']); | ||
const prevClusterConfig = pick(prevDomain.elasticsearchClusterConfig, ['InstanceCount', 'InstanceType']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mstn What is behind this?
are there any other "options" that we would need to compare under ebsOptions
and elasticsearchClusterConfig
?
The problem is when a new es domain is created. It is new for the "local" state, but it might exist on the AWS account (different project, different env...). If it is already present in the local state, then the existing name is used. As far as I can see, the usual approach is to add a random string to the name. |
@bboure perhaps we should hold on. I think they changed how resource names are created. Previously, in many cases, a random string was added to the given name (dynamodb, lambdas, buckets...). Now, the given name is not changed. Let me understand what is the official way. |
ok thanks @mstn |
Hi, I added a random string to component name in order to avoid name clashing. I followed the same approach as for the "official" dynamodb component.